#808 Add Cors feature #849

rserj · 2017-06-26T01:52:24Z

Feature allows to make Cross Origin Requests for Open Id, please see #808 ticket for more information

dnfclas · 2017-06-26T01:52:29Z

@rserj,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

anoordende

A great start, thanks rserj! I've made a few comments intended to find the most restrictive policy. However, regardless of my comments and even though I like the "config free", automatic setup of the policy based on audiences, I'm leaning more and more towards just having policy names defined in modules and the actual configuration left to appsettings.json:


    "Cors": {
        "Policies": [
            {
                "Name": "Orchard.OpenId.OpenIdConnectCorsPolicy",
                "Origins": [ "https://my.externaldomain.com" ],
                "Methods": [ "GET", "POST", "OPTIONS" ],
                "Headers": [ "accept", "content-type", "authorization" ],
                "PreflightMaxAge": 86400
            },
            {
                "Name": "Orchard.SomeOtherModule.SomeOtherCorsPolicy",
                "Origins": [ "https://my.externaldomain.com" ],
                "Methods": [ "GET", "POST", "PATCH", "OPTIONS" ],
                "Headers": [ "accept", "content-type", "authorization" ],
                "AllowCredentials": true
                "PreflightMaxAge": 43200
            }
        ]
    }

anoordende · 2017-06-26T19:05:42Z

src/OrchardCore.Modules/Orchard.OpenId/Controllers/AccessController.cs

@@ -24,6 +26,7 @@
 namespace Orchard.OpenId.Controllers
 {
 [Authorize]
+ [EnableCors(Constants.OpenIdConnectPolicy)]


Is there a way of restricting this to only those actions that may require CORS?

Yeh, I will apply it per Action which requires CORS, thank you

I agree with @anoordende. We need to make sure CORS is only applied to non-interactive endpoints, which excludes both the authorization and logout endpoints.

Not doing that may represent a security risk (e.g an evil application could send a XHR request to the authorization endpoint with all the authentication cookies automatically appended to the headers, which would be terribad)

anoordende · 2017-06-26T19:39:53Z

src/OrchardCore.Modules/Orchard.OpenId/Services/OpenIdCorsConfiguration.cs

+ .WithOrigins(openIdSettings.Audiences.ToArray())
+ .AllowAnyHeader()
+ .AllowAnyMethod()
+ .AllowCredentials()


If this is required for any of the flows, we should consider configuring the policy based on the enabled flows.

Hmm, I didn't think about flows.
If Admin set some external Audience, He assuming that External host can connect to API. However, I missed one case, where Audience will be the same as current host

If you are providing tokens for accessing resources on this site Audiences should be equal to Authority url

Probably we should just check if Audience equals to current Host then ignore it?

Flows based on redirects don't require Cors for the AccessController, only for the UserInfo controller. We are also assuming that every configured App requires UserInfo. So, we'd have to enable/disable Cors at the App level (or for each audience) and have 2 policies: one for the AccessController and another for the UserController. Also, not all flows require AllowCredentials.

anoordende · 2017-06-26T19:40:23Z

src/OrchardCore.Modules/Orchard.OpenId/Services/OpenIdCorsConfiguration.cs

+ if (openIdSettings.Audiences != null && openIdSettings.Audiences.Any())
+ options.AddPolicy(Constants.OpenIdConnectPolicy, builder => builder
+ .WithOrigins(openIdSettings.Audiences.ToArray())
+ .AllowAnyHeader()


Most restrictive policy should be applied, ie. only the verbs and headers that are required, something along the lines of GET, POST, OPTIONS and accept, content_type and authorization.

Agree, Will do

kevinchalet · 2017-06-27T18:15:29Z

Cool stuff happening here! 👍

Two remarks, tho':

You shouldn't use OpenIdSettings.Audiences for the trusted origins. Audiences is only supposed to contain the list of "resources" a client application is allowed to query, not the URLs of the clients that act as JS HTTP clients (it should probably renamed to Resources). Consider using a separate list. Alternatively, we could consider adding the list directly under each application, and resolve the trusted origins dynamically, using OpenIdApplicationStore.
Doing CORS at the controller/action level (with [EnableCors]) is likely not the best option for at least 3 reasons:
- API endpoints like the discovery endpoints (configuration + JWKS) are directly managed by OpenIddict and don't have dedicated controllers/actions. Yet, JS apps should be able to use them.
- Even if you have an action for the token endpoint, requests that are directly rejected by OpenIddict (e.g because they lack a required parameter, or because client authentication failed) won't get the CORS response headers and the JS apps that receive errored responses won't be able to read them.
- OpenIddict (well, the OIDC server behind OpenIddict to be exact) is quite zealous and will automatically reject HTTP requests that don't use GET or POST (depending on the endpoint). Concretely, this means that preflight requests will be rejected by OpenIddict before they have a chance to be handled by MVC's CORS filter.

For these reasons, you should instead consider implementing CORS at the middleware level.

/cc @sebastienros

anoordende · 2017-06-28T09:13:39Z

Thanks @PinpointTownes, great explanation.

That does raise a very fundamental question, unless I'm mistaken, this means throughout Orchard we cannot use MVC Cors (they are mutually exclusive). By default, we can only apply a single policy using the middleware UseCors, so we'd need to implement ICorsPolicyProvider to select the policy based on the HttpContext route dictionary.

Thinking out loud:
Implement ICorsPolicyProvider in the new Cors module, for example an AppSettingsCorsPolicyProvider that loads the policies from appsettings:

    "Cors": {
        "Policies": [
            {
                "Name": "Orchard.OpenId.UserInfo",
                "Origins": [ "https://my.externaldomain.com" ],
                "Methods": [ "GET", "POST", "OPTIONS" ],
                "Headers": [ "accept", "content-type", "authorization" ],
                "PreflightMaxAge": 86400
            },
            {
                "Name": "Orchard.SomeOtherModule.SomeController.Index",
                "Origins": [ "https://my.externaldomain.com" ],
                "Methods": [ "GET", "POST", "PATCH", "OPTIONS" ],
                "Headers": [ "accept", "content-type", "authorization" ],
                "AllowCredentials": true
                "PreflightMaxAge": 43200
            }
        ]
    }

And something along the lines of:

        public static IServiceCollection AddCors(this IServiceCollection services, IConfiguration configuration) {
            var cors = new CorsOptions();
            configuration.GetSection("Cors").Bind(cors);
            services.AddCors(options => {
                foreach (var policy in cors.Policies) {
                    options.AddPolicy(policy.Name, config => {
                        config
                            .WithOrigins(policy.Origins)
                            .WithMethods(policy.Methods)
                            .WithHeaders(policy.Headers)
                            .SetPreflightMaxAge(TimeSpan.FromSeconds(policy.PreflightMaxAge));
                    });
                }
                options.DefaultPolicyName = cors.DefaultPolicy;
            });
            return services;
        }

The naming of the policies can follow a pattern that identifies the requests to apply each policy on, increasingly more specific (or conversely, more generic), based on Area, Controller, Action. For example, the policy provider would look for a policy for /Orchard.OpenId/UserInfo/Me like this:

Orchard.OpenId.UserInfo.Me
Orchard.OpenId.UserInfo
Orchard.OpenId
Orchard
Default
(Deny)

Lastly, having gone full circle, I'm really not so sure that the OpenId module should generate these policies dynamically. I'd much prefer a single point of truth and very granular control instead of assumptions by modules.

rserj · 2017-06-28T17:25:06Z

If we decided stick with Middleware approach, probably we should use MVC RouteTemplate (eg {controller}/{Action}/{arg1}/{arg2}) instead of "Name"?
In such case Do we need to fall-back request to the parent policy if current policy's route didn't mach?
(like, ActionPolicy->ControllerPolicy->Deny)

Basicaly it means we are trying implement the same filtration approach which we have in MVC but in middleware level.

I see two big conserns

Every time Admin, creates, update OpenId Application he should also go to the configuration file and manualy change origins.
Unlike MVC Filters ([EnableCors]), Middleware will be executing in each request, we may have perfomance problems

I'm not an export in OpenIddict but I tried "Password flow" with MVC Origin Filtration ([EnableCors]) with Cross Origin requests, I could retrieve Token and Discovery End-points worked as well.
Probably we should stick With MVC Filtration approach? All we have todo add Origins configuration per OpenId Application.

anoordende · 2017-06-28T17:50:42Z

I just made a quick PoC in one of our internal projects using the ICorsPolicyProvider, completely removing attributes and dynamically selecting the policy, albeit using policy names composed of request path + method instead of Area/Controller/Action. It does provide the granularity and control I'm seeking.

This is an alternative approach to the current PR, but if anyone's interested, I can put this in a separate PR over the next few days so we can compare the two, pick & mix and arrive at a solution to move forward with. Simply 👍 or 👎 me here :)

anoordende · 2017-06-28T18:12:54Z

PS: Sorry @rserj our comments got cross-wired.

The point is that we need to move this out of MVC, as it is not available at this stage in the pipeline,
Yes, it needs to fall back all the way to denying the Cors request.
Once the settings-driven approach is implemented, which I feel will benefit all of Orchard, we can add the "dynamic" per-app origins from OpenId (and any other module that needs it), though probably the AppSettings would always override. To me this is just an administrative nice-to-have, but if it's important to you, then it may be important to others.
I doubt there's much of a noticeable difference in performance between declarative and programmatic, after all, it uses the same internals. But this we can measure :)

rserj · 2017-07-04T06:35:27Z

I added AllowedOrigins Property to OpenId Application and made some changes according to your notes.
Please take a look, thanks

rserj · 2017-07-04T06:53:15Z

I have one concern here, In case if I made changes to OpenId Application's AllowedOrigins, in order to changes take effect, I have to restart Tenant, I guess I have to Warn about it as we do in global OpenId settings by showing Warning message, what do you think?

Ermesx · 2017-07-14T22:29:05Z

src/OrchardCore.Modules/Orchard.OpenId/ModelBinders/UrlsAttribute.cs

+ {
+ var valueAsString = valueProviderResult.FirstValue;
+
+ if (string.IsNullOrEmpty(valueAsString))


This if could be removed.

removed, thanks

Jetski5822 · 2017-07-16T22:58:44Z

src/OrchardCore.Modules/Orchard.OpenId/Services/OpenIdCorsConfiguration.cs

+ && app.AllowedOrigins != null && app.AllowedOrigins.Any())
+ .SelectMany(app => app.AllowedOrigins)
+ .ToArray();
+ if (!appOrigins.Any()) return;


if (appOrigins.Count == 0) { return; }

Jetski5822 · 2017-07-16T22:59:11Z

src/OrchardCore.Modules/Orchard.OpenId/Services/OpenIdCorsConfiguration.cs

+ public void Configure(CorsOptions options)
+ {
+ var openIdApplications = _openIdApplicationStore.GetAllApps().GetAwaiter().GetResult();
+ if (openIdApplications == null || !openIdApplications.Any()) return;


.Count == 0

Jetski5822 · 2017-07-21T19:29:17Z

src/OrchardCore/OrchardCore.Cms/OrchardCore.Cms.csproj

@@ -22,6 +22,7 @@
 -->

 <ItemGroup>
+ <ProjectReference Include="..\..\OrchardCore.Modules\Orchard.Cors\Orchard.Cors.csproj" />


Can you add PrivateAssets="none" here please

rserj · 2017-07-25T01:51:19Z

Nicholas and Wojciech I made commit regarding to your reviews, thanks

anoordende · 2017-07-31T10:40:31Z

Sorry, holidays. Will get back to this thread shortly.

rserj · 2017-11-25T20:57:46Z

I'll close this PR, better to separate CORS from "OpenId Application" settings, please see discussion in
#854

OrchardCMS#808 Add Cors feature

460dc0b

dnfclas added the cla-already-signed label Jun 26, 2017

anoordende reviewed Jun 26, 2017

View reviewed changes

rserj added 2 commits July 3, 2017 09:57

OrchardCMS#808 Add Allowed Origins per OpenId Application

92e9e21

OrchardCMS#808 Add condition, if has any Origins

a51d404

OrchardCMS#808 Change registration scope

4cb7755

Ermesx reviewed Jul 14, 2017

View reviewed changes

rserj mentioned this pull request Jul 15, 2017

OpenId Audience validation failed #870

Closed

Jetski5822 reviewed Jul 16, 2017

View reviewed changes

Jetski5822 reviewed Jul 21, 2017

View reviewed changes

OrchardCMS#808 Fixes after Nicholas and Wojciech reviews

2323dfb

infofromca mentioned this pull request Sep 26, 2017

Orchard 2 as backend for a SPA #1004

Closed

rserj closed this Nov 25, 2017

petedavis mentioned this pull request Nov 13, 2018

#854 Security Headers #1680

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#808 Add Cors feature #849

#808 Add Cors feature #849

rserj commented Jun 26, 2017 •

edited

Loading

dnfclas commented Jun 26, 2017

anoordende left a comment •

edited

Loading

anoordende Jun 26, 2017

rserj Jun 26, 2017

kevinchalet Jun 27, 2017 •

edited

Loading

anoordende Jun 26, 2017

rserj Jun 26, 2017

anoordende Jun 27, 2017

anoordende Jun 26, 2017

rserj Jun 26, 2017

kevinchalet commented Jun 27, 2017

anoordende commented Jun 28, 2017

rserj commented Jun 28, 2017

anoordende commented Jun 28, 2017

anoordende commented Jun 28, 2017 •

edited

Loading

rserj commented Jul 4, 2017

rserj commented Jul 4, 2017

Ermesx Jul 14, 2017

rserj Jul 25, 2017

Jetski5822 Jul 16, 2017 •

edited

Loading

Jetski5822 Jul 16, 2017

Jetski5822 Jul 21, 2017

rserj commented Jul 25, 2017

anoordende commented Jul 31, 2017

rserj commented Nov 25, 2017 •

edited

Loading

#808 Add Cors feature #849

#808 Add Cors feature #849

Conversation

rserj commented Jun 26, 2017 • edited Loading

dnfclas commented Jun 26, 2017

anoordende left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Jun 27, 2017

anoordende commented Jun 28, 2017

rserj commented Jun 28, 2017

anoordende commented Jun 28, 2017

anoordende commented Jun 28, 2017 • edited Loading

rserj commented Jul 4, 2017

rserj commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jetski5822 Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rserj commented Jul 25, 2017

anoordende commented Jul 31, 2017

rserj commented Nov 25, 2017 • edited Loading

rserj commented Jun 26, 2017 •

edited

Loading

anoordende left a comment •

edited

Loading

kevinchalet Jun 27, 2017 •

edited

Loading

anoordende commented Jun 28, 2017 •

edited

Loading

Jetski5822 Jul 16, 2017 •

edited

Loading

rserj commented Nov 25, 2017 •

edited

Loading